fix: generate UUID for request confirmation events#559
fix: generate UUID for request confirmation events#559code2gold wants to merge 1 commit intogoogle:mainfrom
Conversation
generateRequestConfirmationEvent() creates Event structs without setting the ID field, resulting in empty string IDs. When persisted to a database with a composite primary key that includes ID, the second confirmation event causes a duplicate key violation. Set ID via uuid.NewString() to match session.NewEvent() behavior.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a bug where request confirmation events were generated without a UUID, leading to database primary key violations. The fix is simple and effective, adding uuid.NewString() to the event creation. The inclusion of a new unit test, TestGenerateRequestConfirmationEvent_SetsEventID, is excellent; it thoroughly verifies that the event ID is set and that subsequent calls produce unique IDs, preventing future regressions. The changes are well-contained and the pull request description is clear and comprehensive. Overall, this is a high-quality contribution.
| return &session.Event{ | ||
| ID: uuid.NewString(), | ||
| InvocationID: invocationContext.InvocationID(), | ||
| Author: invocationContext.Agent().Name(), | ||
| Branch: invocationContext.Branch(), | ||
| LLMResponse: model.LLMResponse{ | ||
| Content: &genai.Content{ | ||
| Parts: parts, | ||
| Role: genai.RoleModel, | ||
| }, | ||
| Content: content, | ||
| }, | ||
| Timestamp: time.Now(), | ||
| LongRunningToolIDs: longRunningToolIDs, | ||
| Actions: session.EventActions{}, | ||
| } |
There was a problem hiding this comment.
Thank you for fixing this.
I'd propose one change: could you please use session.NewEvent function here instead of the raw struct initialization?
This way, if the NewEvent function ever gets modified, this place will be updated with it, keeping code consistent and safe automatically.
| event := session.NewEvent(invocationContext.InvocationID()) | |
| event.Author = invocationContext.Agent().Name() | |
| event.Branch = invocationContext.Branch() | |
| event.LLMResponse = model.LLMResponse{Content: content} | |
| event.LongRunningToolIDs = longRunningToolIDs | |
| return event |
4 participants
Fixes #558
Summary
generateRequestConfirmationEvent()createssession.Eventstructs without setting theIDfield, resulting in empty string IDs. When persisted to a database backend, the composite primary key(ID, AppName, UserID, SessionID)causes a duplicate key violation on the second HITL confirmation event within the same session.Fix: Add
ID: uuid.NewString()to the Event struct initialization, consistent withsession.NewEvent().Changes
internal/llminternal/functions.go: AddID: uuid.NewString()anduuidimportinternal/llminternal/base_flow_test.go: Add test verifying generated events have non-empty, unique IDsTesting Plan
Unit test added:
TestGenerateRequestConfirmationEvent_SetsEventID— verifies the generated event has a non-empty ID, and calling twice produces unique IDsManual E2E verification:
database.NewSessionService(PostgreSQL via GORM)duplicate key value violates unique constraint "events_pkey"